Skip to content

fix desktop navigation boundary#340

Merged
hqhq1025 merged 3 commits into
OpenCoworkAI:mainfrom
snowopsdev:fix/desktop-navigation-boundary
May 19, 2026
Merged

fix desktop navigation boundary#340
hqhq1025 merged 3 commits into
OpenCoworkAI:mainfrom
snowopsdev:fix/desktop-navigation-boundary

Conversation

@snowopsdev
Copy link
Copy Markdown
Contributor

Summary

  • Add a main-window navigation policy that keeps the Electron app document on the trusted app URL/origin.
  • Block untrusted top-level will-navigate and main-frame will-redirect attempts, while preserving the existing allowlisted external-open behavior.
  • Route Markdown links through a safe renderer helper so workspace Markdown cannot rely on default top-level navigation.
  • Add focused tests for trusted navigation classification and Markdown link classification.

Closes #339

Why

Workspace Markdown is untrusted content rendered inside the desktop app chrome. The app already denied window.open attempts, but top-level navigation and redirects also need to be guarded so a clicked Markdown link cannot replace the trusted app document in the main BrowserWindow.

Test Plan

  • pnpm --filter @open-codesign/desktop exec vitest run src/main/navigation-policy.test.ts src/renderer/src/lib/markdown-links.test.ts src/main/open-external.test.ts
  • pnpm --filter @open-codesign/desktop typecheck
  • pnpm --filter @open-codesign/desktop test
  • pnpm typecheck
  • pnpm lint
  • pnpm test

@github-actions github-actions Bot added the area:desktop apps/desktop (Electron shell, renderer) label May 12, 2026
…into fix/desktop-navigation-boundary

# Conflicts:
#	apps/desktop/src/renderer/src/components/FilesTabView.tsx
@hqhq1025 hqhq1025 added the bot-rerun Temporary label to rerun automation label May 17, 2026
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review mode: initial

Findings

  • [Minor] Missing changeset for user-visible security behavior. The PR adds main-window navigation blocking and a new Markdown link handler that changes how external links behave in the workspace file viewer. These are user-visible changes and should have a changeset per the project conventions (AGENTS.md, CLAUDE.md).
    Suggested fix: run pnpm changeset add --type patch --message "fix(desktop): add main-window navigation policy and safe Markdown link handler" and commit the generated file.

  • [Nit] The MarkdownLink component in FilesTabView.tsx silently drops rel, target, and other anchor attributes passed via ...props when the link is blocked (rendered as <span>). Consider preserving title or className for styling continuity, though the blocked state is intentionally non-interactive so this is cosmetic.

Summary
The PR correctly addresses the security concern: untrusted Markdown links in workspace files can no longer navigate the main Electron window away from the trusted app origin. The main-process will-navigate/will-redirect handlers block top-level navigations that don't match the expected dev-server or packaged-file origin. The renderer-side classifyMarkdownHref function routes HTTPS links through the existing openExternal IPC (which opens the system browser) and blocks all other protocols, relative paths, and anchors except # fragments. Tests cover both new modules thoroughly. The PR does not introduce new dependencies, violates no hard constraints, and satisfies the stated goal in the issue (assuming issue #339 matches the described scope). No blockers or major issues found. Only a minor changeset gap and a cosmetic nit.

Testing

  • navigation-policy.test.ts covers same-origin dev, remote rejection, cross-port rejection, hash-on-file, different-file rejection, and malformed URLs.
  • markdown-links.test.ts covers anchors, HTTPS external, whitespace normalization, relative paths, and unsafe protocols (javascript, file, http).
  • Integration with Electron events is not tested headlessly, but the pure functions are well-covered. Suggested to run pnpm test and pnpm typecheck on the apps/desktop package to confirm no regressions.

Open-CoDesign Bot

@hqhq1025 hqhq1025 removed the bot-rerun Temporary label to rerun automation label May 17, 2026
@github-actions github-actions Bot added the docs Documentation label May 19, 2026
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review mode: follow-up after new commits

Findings

No new issues introduced beyond the previous review. The missing changeset has been added (.changeset/desktop-navigation-boundary.md), resolving the prior minor finding. The cosmetic nit about dropped anchor attributes remains unchanged but is non-blocking.

Summary
The PR correctly implements the security hardening described in the linked issue. New files (navigation-policy.ts, markdown-links.ts) and their tests are well-structured and cover the expected cases. Main-window navigation policy blocks untrusted top-level navigations and redirects, while the Markdown link handler routes HTTPS links through the safe openExternal IPC. Both code changes are consistent with the project's security model and architectural constraints (no new deps, no silent fallbacks, no direct SDK imports). The PR is ready to merge.

Testing

  • Unit tests for both new modules are thorough and pass (as verified by the CI run associated with this commit). No additional tests required.

Open-CoDesign Bot

@hqhq1025 hqhq1025 merged commit 8efd7e3 into OpenCoworkAI:main May 19, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:desktop apps/desktop (Electron shell, renderer) docs Documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Harden desktop navigation boundary for workspace Markdown links

2 participants